Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Huawei AC-Charger (Pylontech) BMS connection #767

Merged

Conversation

MalteSchm
Copy link

@MalteSchm MalteSchm commented Mar 18, 2024

This PR connects the Huawei AC-Charger with the Battery BMS. It specifically introduces the following features:

When the BMS issues an charge immediate request and the charger is in internal automatic control mode then charging is started with full configured power. This behavior enabled / disabled on the GUI

The Huawei charger will respect BMS current limits now when charging and limit the current taking the BMS limit and an estimate of other DC sources into account.

I did some testing but would like to monitor this a bit over the course of the coming days. Marking this PR as draft for now.

The request is in #723

@MalteSchm
Copy link
Author

MalteSchm commented Mar 25, 2024

I did some testing of this PR. Running a test poses some difficulties as the BMS requested current drops quickly and voltage limits also kick in.
Anyway I did test and believe that the code works as expected. Specifically I saw the maximum output current of the AC charger follow the BMS limit.
I further verified that an Pylontech charge immediately alarm starts up the Huawei charger with full power.

Moving to review / merge phase

@MalteSchm MalteSchm marked this pull request as ready for review March 25, 2024 14:54
@schlimmchen
Copy link
Member

Alright 💪

I am confused that you did chose not to use the method "needsCharging()" I implemented for you. Was that a misunderstanding or why did you chose to use a different method name? I find "getImmediateChargingRequest()" is very specific to the Pylontech battery stats, however, the functionality can be applied with other BMSs as well. Hence the more general name "needsCharging()".

@MalteSchm
Copy link
Author

Hi Bernhard,

I considered to use the method name that you had in your implementation. However the naming scheme in BatteryStats.h mostly uses the typical get.../set... for getters and setters. needsCharging does not. I implemented a few getters and thought it would be good to apply this convention.

Since I anyhow had to change the name I went for something that I considered consistent with how the naming was used in other parts of the code. I agree that this aligns naming with Pylontech. But I considered this better for understanding and code readability.

That being said: I'm not so hung up on the actual name. But I think it should start with get.... I.e. the method should at least be called getNeedsCharging().

Let me know if I should change this

Malte

@schlimmchen
Copy link
Member

I'm not so hung up on the actual name.

Same here.

It seems this needs some work rebasing due to changes to the locales, which I will take care of. I will also clean up the remnants of "needsCharging()". I will not be able to test this due to lack of respective hardware.

This change logically connects the AC-Charger with the BMS to add BMS
initiated emergency charging and respecting BMS current limits.
@schlimmchen schlimmchen force-pushed the huawei_emergency_charge_support branch from 574e3ef to 6958119 Compare April 2, 2024 14:29
@schlimmchen
Copy link
Member

Rebased onto current helgeerbe/development, solved conflicts, cleaned up "needsCharging()".

@schlimmchen schlimmchen merged commit 8abf614 into hoylabs:development Apr 2, 2024
8 checks passed
Copy link

github-actions bot commented May 3, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants